bpo-1154351: Add get_current_dir_name() to os module by bradengroom · Pull Request #10117 · python/cpython

bradengroom · 2018-10-26T02:40:51Z

Co-authored-by: Marc Adam Anderson marc@marcadam.com .

https://bugs.python.org/issue1154351

Co-authored-by: Marc Adam Anderson <marc@marcadam.com> .

serhiy-storchaka · 2018-10-26T07:36:48Z

Doc/library/os.rst

+    function is identical to :func:`getcwd()` on systems that do **not**
+    support the ``PWD`` environment variable.
+
+    .. availability:: Unix.


This function is available on all platforms.

Actually, while this is undoubtably an ancient link ( https://www.gnu.org/software/gnulib/manual/html_node/get_005fcurrent_005fdir_005fname.html ) I do not believe this function is available in all POSIX environments. It may be common in GNU runtime environments.

FYI: I just checked AIX and found the routine name "exists" in AIX <unistd.h>, but there is no documentation. So, it seems it may be visible on AIX, even if they have never taken the time to a is adding dd it to the getcwd() manual page.

Confusion #1 in adding this

serhiy-storchaka · 2018-10-26T07:37:59Z

Lib/os.py

+    except KeyError:
+        return cwd
+
+    cwd_stat, pwd_stat = map(stat, [cwd, pwd])


Would be clearer just to write two stat calls.

serhiy-storchaka · 2018-10-26T07:38:39Z

Lib/os.py

+
+    cwd_stat, pwd_stat = map(stat, [cwd, pwd])
+
+    if (cwd_stat.st_dev == pwd_stat.st_dev and


Maybe use samefile()?

serhiy-storchaka · 2018-10-26T07:40:34Z

Lib/test/test_os.py

+        # os.getcwd() always returns the dereferenced path
+        with support.temp_cwd(self.tmp_dir):
+            os.chdir(self.tmp_dir)
+            self.assertEqual(self.tmp_dir, os.getcwd())


Swap arguments. The first argument is an actual value, the second argument is an expected value.

serhiy-storchaka · 2018-10-26T07:48:10Z

Lib/test/test_os.py

+                self.assertEqual(self.tmp_dir, os.getcwd())
+            with mock.patch.dict('os.environ', {'PWD': self.tmp_lnk}):
+                self.assertEqual(self.tmp_dir, os.getcwd())
+            os.unlink(self.tmp_lnk)


The link will be leaked in the case of failure. It is better to use addCleanup(). Add the following line just before creating a link.

self.addCleanup(support.unlink, self.tmp_lnk)

aixtools · 2018-10-26T14:08:09Z

Lib/os.py

+
+    cwd_stat, pwd_stat = map(stat, [cwd, pwd])
+
+    if (cwd_stat.st_dev == pwd_stat.st_dev and


So, confusion #2 -, assuming that get_current_dir_name() is supported by the platform why is that not just being called rather than rewriting it? I admit my confusion comes from the lack of platform documentation, but why so hard? Wouldn't calling the "platform" implementation be more efficient than writing this in "pure" python.

More simply, why not use the platform implementation, perhaps adding it to posixmodule.c rather than here?

bradengroom · 2018-10-27T05:47:44Z

Lib/test/test_os.py

+            os.symlink(self.tmp_dir, self.tmp_lnk, True)
+            os.chdir(self.tmp_lnk)
+            if os.name == 'nt':
+                # windows doesn't dereference the path


I'm not sure if this is a bug or the expected behavior. I haven't used windows in quite some time, but this is the behavior I'm seeing in CI.

bradengroom · 2018-10-27T05:49:22Z

Lib/os.py

+    cwd = getcwd()
+
+    if name == 'nt':
+        return cwd


I added this so that we don't look at the PWD environment variable on windows. I believe this matches the behavior documented.

bradengroom · 2018-10-27T05:53:48Z

@serhiy-storchaka I've addressed your feedback and the tests are passing now.

serhiy-storchaka

Please add also an entry in What's New.

serhiy-storchaka · 2018-10-27T05:44:23Z

Doc/library/os.rst

+.. function:: get_current_dir_name()
+
+    Return a string representing the current working directory taking into
+    consideration the users ``PWD`` environment variable if it exists. This is


Use :envvar:?

serhiy-storchaka · 2018-10-27T05:45:09Z

Doc/library/os.rst

+
+    Return a string representing the current working directory taking into
+    consideration the users ``PWD`` environment variable if it exists. This is
+    opposed to :func:`getcwd()` which dereferences symlinks in the path. This


Remove (). It is added automatically.

serhiy-storchaka · 2018-10-27T05:49:25Z

Lib/os.py

+    the *PWD* environment variable.
+    """
+    cwd = getcwd()
+


I think empty lines in this functions are redundant. Empty lines inside functions are used for separating groups of statements in large functions, but this function is enough small and simple. And will be smaller after removing empty lines.

serhiy-storchaka · 2018-10-27T05:50:22Z

Lib/os.py

+    """
+    cwd = getcwd()
+
+    if name == 'nt':


Why this is needed?

I think you were performing review as I was writing a comment :)
#10117 (comment)

This function is documented with:

This function is identical to :func:getcwd on systems that do not support the :envvar:PWD environment variable.

Please correct me if I'm wrong since I'm less familiar with Windows, but I don't believe it uses the PWD environment variable in any way. I added that check so that we would not incorrectly change behavior from getcwd() if the user does set the PWD on Windows for some reason.

in git bash on windows:

Anthony@AnthonysDesktop MINGW64 ~ $ python -c 'import os; print(os.getenv("PWD"))' C:/Users/Anthony

@asottile
Thanks. Looks like my memory was incorrect. I'll fix this up.

I'm actually not sure this function is useful/a good idea -- it's entirely dependent on whether python is executed in the context of an interactive shell that sets or doesn't set PWD.

I've commented on the bpo issue indicating that as well.

serhiy-storchaka · 2018-10-27T05:53:57Z

Misc/NEWS.d/next/Library/2018-10-25-19-44-42.bpo-1154351.KhXLos.rst

@@ -0,0 +1 @@
+Add get_current_dir_name() to the os module.


"Patch by ..."

bradengroom · 2018-10-27T06:32:54Z

@serhiy-storchaka Thanks for the quick feedback. I addressed your comments here:
17386e1
#10117 (comment)

vstinner

Right now I have no opinion on the feature itself, but if we decide to add it, it should be added to the shutil module instead. os is a thin wrapper to C function, whereas shutil are functions based on the os module but adding "Python logic".

vstinner

I don't understand the purpose of this function. I suggest to reject this PR and close https://bugs.python.org/issue1154351

If someone needs this function, it can easily be reimplemented and copy/paste from the issue or this PR.

If someone really wants this feature to be added to Python stdlib, we need realistic use cases. Not just "it would be nice to have this function".

bedevere-bot · 2018-10-28T18:05:41Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

morell5 · 2019-09-09T07:18:50Z

@vstinner , may you make a key decision about this PR: close or provide requirements for merging the PR? The development of the task has stopped because there are no instructions from the core developer. As for me, I don't understand further actions on this task.

Also I don't understand what "requested changes" bedeavere-bot was talking about? Where I can find these "requested changes"?

vstinner · 2019-09-09T10:11:18Z

The os module is thin wrappers to functions of the libc or even system calls. We don't implement heuristic using 2 variants of the same feature: there shutil module is there for that. Compare os.get_terminal_size() to shutil.get_terminal_size() for example.

I close the PR to add os.get_current_dir_name(): IMHO it's more a feature for the shutil module.

bpo-1154351: add get_current_dir_name() to os module

1daae93

Co-authored-by: Marc Adam Anderson <marc@marcadam.com> .

the-knights-who-say-ni added the CLA signed label Oct 26, 2018

bedevere-bot added the awaiting review label Oct 26, 2018

bradengroom changed the title ~~Add get_current_dir_name() to os module~~ bpo-1154351: Add get_current_dir_name() to os module Oct 26, 2018

bradengroom added 3 commits October 25, 2018 19:46

fixup! bpo-1154351: add get_current_dir_name() to os module

ee4c4f6

fixup! bpo-1154351: add get_current_dir_name() to os module

f836691

fixup! bpo-1154351: add get_current_dir_name() to os module

89e1984

serhiy-storchaka reviewed Oct 26, 2018

View reviewed changes

aixtools reviewed Oct 26, 2018

View reviewed changes

bradengroom added 4 commits October 26, 2018 21:31

fixup! bpo-1154351: add get_current_dir_name() to os module

4bfb33c

fixup! bpo-1154351: add get_current_dir_name() to os module

963659a

fixup! bpo-1154351: add get_current_dir_name() to os module

cc1735f

fixup! bpo-1154351: add get_current_dir_name() to os module

d49fdf3

bradengroom commented Oct 27, 2018

View reviewed changes

serhiy-storchaka reviewed Oct 27, 2018

View reviewed changes

fixup! bpo-1154351: add get_current_dir_name() to os module

17386e1

vstinner reviewed Oct 28, 2018

View reviewed changes

vstinner requested changes Oct 28, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Oct 28, 2018

vstinner closed this Sep 9, 2019


		cwd_stat, pwd_stat = map(stat, [cwd, pwd])

		if (cwd_stat.st_dev == pwd_stat.st_dev and

Uh oh!

Conversation

bradengroom commented Oct 26, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradengroom commented Oct 27, 2018

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradengroom commented Oct 27, 2018

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Oct 28, 2018

Uh oh!

morell5 commented Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Sep 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

bradengroom commented Oct 26, 2018 •

edited by bedevere-bot

Loading

morell5 commented Sep 9, 2019 •

edited

Loading